Skip to content

Conversation

@Mab879
Copy link
Member

@Mab879 Mab879 commented Jan 28, 2026

Description:

Review each commit for details.

Rationale:

Fixes #13690

Review Hints:

Run automatus tests for

@Mab879 Mab879 added this to the 0.1.80 milestone Jan 28, 2026
@Mab879 Mab879 added the bugfix Fixes to reported bugs. label Jan 28, 2026
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Jan 28, 2026
@openshift-ci
Copy link

openshift-ci bot commented Jan 28, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@Mab879 Mab879 marked this pull request as ready for review January 28, 2026 18:57
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Jan 28, 2026
@github-actions
Copy link

github-actions bot commented Jan 28, 2026

ATEX Test Results

Test artifacts have been submitted to Testing Farm.

Results: View Test Results
Workflow Run: View Workflow Details

This comment was automatically generated by the ATEX workflow.

@jan-cerny jan-cerny self-assigned this Jan 29, 2026
Copy link
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How have you reproduced the linked issue and how have you verified that this PR fixes the issue?

I can't reproduce the issue using Automatus. However, I can see it reproduced using contest, eg. in the daily productization run today.

I have run /per-rule/oscap/from-env contest test in a custom productization pipeline on this PR and the issue still persists.

What I find suspicious in the HTML report from the initial scan is that the test scenario banner_etc_issue_disa_dod_short.pass.sh doesn't seem to modify the /etc/issue file.


elif remediation_type == "bash":
pattern = r'\(bash-populate\s*(\S+)\)'
pattern = r'\(bash-populate\s*([a-zA-Z0-9_]+)\)'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed? It doesn't change the generated data stream. Both old and new version of the shared.sh is matched by the old expression.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't strictly needed but the old regex can get tripped up if the expression ends in )).

@Mab879 Mab879 marked this pull request as draft January 29, 2026 19:22
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Jan 29, 2026
@Mab879
Copy link
Member Author

Mab879 commented Jan 29, 2026

This might actually might be a contest bug in part.

Around here: https://github.com/RHSecurityCompliance/contest/blob/c1620e15ee5c9d79aebb7c72b4edb50718da238c/per-rule/runner.sh#L89-L91 it seems we need to escape the ' for yaml otherwise the apostrophe is getting interpreted as the closing single quote for the variable.

@comps thoughts?

This PR is still needed to fix a couple issues on the content side.

  • banner_etc_issue_disa_dod_short.fail.sh is broken when using Automatus without edda243
  • The & gets double escaped without e40f1e5. Maybe something we can fix also contest side?

The other two commits (001b620 and 37f4731) can be dropped. Before I rebase this PR let's figure what we want to solve where.

Edit: value giving us all the trouble is ^I've[\s\n]+read[\s\n]+&[\s\n]+consent[\s\n]+to[\s\n ]+terms[\s\n]+in[\s\n]+IS[\s\n]+user[\s\n]+agreem't\.$

@jan-cerny
Copy link
Collaborator

The section you linked https://github.com/RHSecurityCompliance/contest/blob/c1620e15ee5c9d79aebb7c72b4edb50718da238c/per-rule/runner.sh#L89-L91 seems to be related to Ansible but we have a problem when running bash remediation.

I still don't understand what causes that the test scenario doesn't insert the string to /etc/issue. The test log suggests that the scenario inserted it there, but then oscap reads the previous string. Can something prevent the test scenario from writing in the /etc/issue file?

@comps
Copy link
Collaborator

comps commented Jan 30, 2026

@Mab879 So there are two things to address:

I remember running into the first one back when I was implementing /per-rule - the fact that content seems to hardcode XML syntax (&) into the YAML, which just seemed wrong to me - we should write YAML as YAML and if XML needs some escaping, we should do that transformation when translating YAML to XML, not pre-escape strings in YAML.

I think that broke a few tests back then (because Automatus was writing datastream files verbatim, with & and everything), but my /per-rule was actually (IMHO correctly) doing the translation. I don't remember how we solved it back then, if at all, or if I simply skipped those tests, or made /per-rule behave like Automatus.

But my view is that & in YAML is a content bug and Automatus was designed to take advantage of that bug, not the other way around.


Regarding the awk - you are right, that is a Contest bug. AWK interprets special characters in gensub().

I tried to play around with match() and substr() which was supposed to be immune to that, but AI lied and those functions also interpret at least \n even if they don't treat & as special.

So, instead, I realized we don't actually need awk or (much slower) Python, and can do it using bash builtins:

while IFS= read -r line; do
  if [[ $line =~ ^([[:space:]]+)$key: ]]; then
    printf '%s%s: %s\n' "${BASH_REMATCH[1]}" "$key" "$new_value"
  else
    printf '%s\n' "$line"
  fi
done

which seems to do the replacement correctly:

$ read -r new_value
^I've[\s\n]+read[\s\n]+&[\s\n]+consent[\s\n]+to[\s\n      ]+terms[\s\n]+in[\s\n]+IS[\s\n]+user[\s\n]+agreem't\.$

$ printf '%s\n' "$new_value"
^I've[\s\n]+read[\s\n]+&[\s\n]+consent[\s\n]+to[\s\n      ]+terms[\s\n]+in[\s\n]+IS[\s\n]+user[\s\n]+agreem't\.$

$ grep login_banner_text: rhel9/playbooks/all/banner_etc_issue.yml
    login_banner_text: ^Authorized[\s\n]+users[\s\n]+only\.[\s\n]+All[\s\n]+activity[\s\n]+may[\s\n]+be[\s\n]+monitored[\s\n]+and[\s\n]+reported\.$

$ while IFS= read -r line; do   if [[ $line =~ ^([[:space:]]+)$key: ]]; then     printf '%s%s: %s\n' "${BASH_REMATCH[1]}" "$key" "$new_value";   else     printf '%s\n' "$line";   fi; done < rhel9/playbooks/all/banner_etc_issue.yml | grep login_banner_text:
    login_banner_text: ^I've[\s\n]+read[\s\n]+&[\s\n]+consent[\s\n]+to[\s\n      ]+terms[\s\n]+in[\s\n]+IS[\s\n]+user[\s\n]+agreem't\.$
--- rhel9/playbooks/all/banner_etc_issue.yml    2026-01-30 10:36:37.365885533 +0100
+++ new 2026-01-30 11:18:46.116887483 +0100
@@ -10 +10 @@
-    login_banner_text: ^Authorized[\s\n]+users[\s\n]+only\.[\s\n]+All[\s\n]+activity[\s\n]+may[\s\n]+be[\s\n]+monitored[\s\n]+and[\s\n]+reported\.$
+    login_banner_text: ^I've[\s\n]+read[\s\n]+&[\s\n]+consent[\s\n]+to[\s\n      ]+terms[\s\n]+in[\s\n]+IS[\s\n]+user[\s\n]+agreem't\.$

I'll create a PR for Contest to fix it.

Note that this solution also isn't perfect as we would ideally escape the new_value for use with YAML, so that a value of ie. blabla: is not interpreted as a YAML key, but a string instead. ... But if I did that, I fear I would run into the same problem as with XML, that Automatus tests pre-escape variable values for use with XML or YAML.

(But, for the record, it could be done with |- and a specific indent specifier, so leading spaces are also preserved, ie.

vars:
    some_var: |-1
     blabla: this is fully verbatim beyond the 1 leading space
other_key: ...

)

@jan-cerny
Copy link
Collaborator

I think that I now know where my confusion comes from. I focused only on the pass test scenario banner_etc_issue_disa_dod_short.pass.sh because that is the one reported in the linked GH issue. And you now found and try to solve a problem with banner_etc_issue_disa_dod_short.fail.sh. Do I understand it correctly?

comps added a commit to RHSecurityCompliance/contest that referenced this pull request Jan 30, 2026
As the comment suggests, awk was interpreting at least \n and &
as special and corrupting the value.

See also
ComplianceAsCode/content#14343 (comment)

Signed-off-by: Jiri Jaburek <[email protected]>
@Mab879
Copy link
Member Author

Mab879 commented Jan 30, 2026

I think that I now know where my confusion comes from. I focused only on the pass test scenario banner_etc_issue_disa_dod_short.pass.sh because that is the one reported in the linked GH issue. And you now found and try to solve a problem with banner_etc_issue_disa_dod_short.fail.sh. Do I understand it correctly?

That is correct.

@comps
Copy link
Collaborator

comps commented Jan 30, 2026

Note that this solution also isn't perfect as we would ideally escape the new_value for use with YAML, so that a value of ie. blabla: is not interpreted as a YAML key, but a string instead. ... But if I did that, I fear I would run into the same problem as with XML, that Automatus tests pre-escape variable values for use with XML or YAML.

Turns out I will have to do it like that, because there are many playbooks with variables like

  vars:
    sshd_max_auth_tries_value: 123

or

  vars:
    var_sshd_set_maxstartups: 10:30:60

or

  vars:
    var_sshd_disable_compression: no

and these get converted to int/dict/bool by YAML when they should be strings, which is possibly what causes a lot of failures with the bash loop approach (which doesn't try to quote anything, unlike the awk).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes to reported bugs. do-not-merge/work-in-progress Used by openshift-ci bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

banner_etc_issue test scenario is using wrong variable

3 participants